Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(fix/solve) Solve ofac.json path issue #244

Merged
merged 4 commits into from
Sep 25, 2024
Merged

(fix/solve) Solve ofac.json path issue #244

merged 4 commits into from
Sep 25, 2024

Conversation

PavelInjective
Copy link
Contributor

@PavelInjective PavelInjective commented Sep 23, 2024

Summary by CodeRabbit

  • Refactor
    • Simplified the logic for constructing the path to the OFAC list, improving efficiency without altering functionality.

Copy link

coderabbitai bot commented Sep 23, 2024

Walkthrough

The changes involve the getOfacListPath function in the client/chain/ofac.go file. The previous implementation's directory traversal logic has been removed, and the function now directly constructs the path to the OFAC list using a new helper function. This streamlining maintains the function's original purpose while simplifying the path construction process.

Changes

File Change Summary
client/chain/ofac.go Modified getOfacListPath function to remove directory traversal logic and directly construct the path to the OFAC list using getFileAbsPath.

Poem

In the code where paths once roamed,
A rabbit found a simpler home.
No more loops to search and seek,
Just a path that's clear and sleek.
Hopping through the lines with glee,
Simplification sets us free! 🐇✨


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between fef78fa and a6ba870.

📒 Files selected for processing (1)
  • client/chain/ofac.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • client/chain/ofac.go

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@PavelInjective PavelInjective changed the title OFAC fix (fix/solve) Solve ofac.json path issue Sep 23, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (1)
client/chain/ofac.go (1)

Line range hint 26-30: Update callers of getOfacListPath

If getOfacListPath is modified to return an error as suggested in the previous comment, the following functions need to be updated to handle this error:

  1. NewOfacChecker
  2. DownloadOfacList

Here are the suggested changes:

For NewOfacChecker:

func NewOfacChecker() (*OfacChecker, error) {
    ofacListPath, err := getOfacListPath()
    if err != nil {
        return nil, fmt.Errorf("failed to get OFAC list path: %w", err)
    }
    checker := &OfacChecker{
        ofacListPath: ofacListPath,
    }
    // ... rest of the function
}

For DownloadOfacList:

func DownloadOfacList() error {
    // ... existing code ...

    ofacListPath, err := getOfacListPath()
    if err != nil {
        return fmt.Errorf("failed to get OFAC list path: %w", err)
    }
    outFile, err := os.Create(ofacListPath)
    if err != nil {
        return err
    }
    // ... rest of the function
}

These changes will ensure proper error handling throughout the OFAC checking process.

Also applies to: 48-52

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 08caf8f and b6de017.

Files selected for processing (1)
  • client/chain/ofac.go (1 hunks)
Additional comments not posted (2)
client/chain/ofac.go (2)

Line range hint 1-93: General improvements for OFAC handling

After reviewing the entire file, here are some suggestions for improvement:

  1. Make the OFAC list URL configurable:
    Instead of hardcoding defaultOfacListURL, consider using an environment variable or a configuration file. This would allow for easier updates and environment-specific URLs.

    var ofacListURL = os.Getenv("OFAC_LIST_URL")
    if ofacListURL == "" {
        ofacListURL = "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/master/wallets/ofac.json"
    }
  2. Use more inclusive terminology:
    Replace IsBlacklisted with a more neutral term like IsRestricted or IsOnSanctionsList.

    func (oc *OfacChecker) IsRestricted(address string) bool {
        return oc.ofacList[address]
    }
  3. Implement periodic updates:
    Consider adding a goroutine that periodically checks for updates to the OFAC list and refreshes the local copy if necessary.

These changes would improve the flexibility, inclusivity, and up-to-date nature of the OFAC checking system.

Let's verify the current update mechanism and usage of the IsBlacklisted function:

#!/bin/bash
# Description: Check for update mechanisms and usage of IsBlacklisted

# Test 1: Look for any update mechanisms
echo "Searching for update mechanisms:"
rg --type go "update.*ofac"

# Test 2: Check usage of IsBlacklisted
echo "Usage of IsBlacklisted:"
rg --type go "IsBlacklisted"

43-43: Simplification introduces potential fragility

The simplified getOfacListPath function removes the previous directory traversal logic, which makes the code cleaner but potentially less robust. Consider the following points:

  1. The function now assumes a fixed directory structure relative to the current working directory. This might break if:

    • The function is called from a different working directory.
    • The project structure changes.
  2. There's no error handling for os.Getwd(), which could fail in certain environments.

To improve robustness, consider:

  1. Using an absolute path or an environment variable to locate the OFAC list file.
  2. Adding error handling for os.Getwd().

Here's a potential improvement:

func getOfacListPath() (string, error) {
    currentDirectory, err := os.Getwd()
    if err != nil {
        return "", fmt.Errorf("failed to get current directory: %w", err)
    }
    return filepath.Join(currentDirectory, "../metadata", ofacListFilename), nil
}

This change would require updating the callers of getOfacListPath to handle the potential error.

Let's verify the usage of getOfacListPath in the codebase:

Verification successful

[/run_scripts]


[/run_scripts]


[/run_scripts]


Path to OFAC list is correctly defined and accessible

The getOfacListPath function accurately points to client/metadata/ofac.json, which exists in the repository. No issues were found with the current implementation.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check usage of getOfacListPath and potential impacts

# Test 1: Find all occurrences of getOfacListPath
echo "Occurrences of getOfacListPath:"
rg --type go "getOfacListPath"

# Test 2: Check if there are any other files in the metadata directory
echo "Files in ../metadata directory:"
fd . "../metadata"

Length of output: 650


Script:

#!/bin/bash
# Description: Check usage of getOfacListPath and potential impacts

# Test 1: Find all occurrences of getOfacListPath
echo "Occurrences of getOfacListPath:"
rg --type go "getOfacListPath"

# Test 2: Check if there are any other files in the metadata directory
echo "Files in ../metadata directory:"
fd . "../metadata"

Length of output: 604


Script:

#!/bin/bash
# Description: Locate all 'metadata' directories and list their contents

# Test 1: Find all directories named 'metadata'
echo "Searching for all 'metadata' directories in the repository:"
fd metadata --type d

# Test 2: For each found 'metadata' directory, list its contents
echo "Listing contents of each 'metadata' directory:"
fd metadata --type d -x ls -la {}

Length of output: 1739


Script:

#!/bin/bash
# Description: Locate the definition of 'ofacListFilename' and verify its usage

# Test 1: Find the definition of 'ofacListFilename'
echo "Searching for the definition of 'ofacListFilename':"
rg --type go "ofacListFilename\s*="

# Test 2: Find all occurrences of 'ofacListFilename' to understand its usage
echo "Occurrences of 'ofacListFilename' in the codebase:"
rg --type go "ofacListFilename"

# Test 3: Verify if 'ofac.json' exists in 'client/metadata/' directory
echo "Checking for 'ofac.json' in 'client/metadata/':"
fd ofac.json client/metadata/

# Test 4: List all 'ofac.json' files in the repository
echo "Listing all 'ofac.json' files in the repository:"
fd ofac.json --type f

Length of output: 1272

Copy link
Collaborator

@aarmoa aarmoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am adding just one comment.

Also @PavelInjective I notice that in this PR you did a force push. I already asked in the past to please avoid force pushes. From now one if a PR includes a force push, I will directly reject it.

currentDirectory = filepath.Dir(currentDirectory)
}
return filepath.Join(filepath.Join(filepath.Join(currentDirectory, "client"), "metadata"), ofacListFilename)
return getFileAbsPath(fmt.Sprintf("../metadata/%s", ofacListFilename))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not make assumptions regarding the directory separator used by the OS. To create a path we should be using path.Join

Copy link
Collaborator

@aarmoa aarmoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me

@PavelInjective PavelInjective merged commit 92ece76 into master Sep 25, 2024
4 checks passed
@PavelInjective PavelInjective deleted the ofac-fix branch September 25, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants